refactor apparmor utils in e2e #84439

danielqsj · 2019-10-28T08:35:00Z

What type of PR is this?

/kind feature
/sig testing

What this PR does / why we need it:

test/e2e/common is a mixture of tests (test/e2e/common/downward_api.go) and utility code (test/e2e/common/events.go). It would be nice to refactor it so that the utility code can be imported without also importing the tests. Most likely a better place for the utility code is under test/e2e/framework.

Which issue(s) this PR fixes:

part of #83937

Special notes for your reviewer:

/cc @pohly

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

mattjmcnaughton

/lgtm

I think this change makes a lot of sense. Would also love someone with more direct ownership of test/e2e to sign-off that framework is the proper home.

Also, would love you mind addressing the golint errors? Thank you!

alejandrox1

Thank you very much for working on this @danielqsj !
Added a couple comments to guide the refactoring into the e2e test framework.
Overall, we want to include any code that may be useful for a user to test behaviour, if there is code that is specific to a test then the code should stay within the test.

A big thing is that function within an e2e test framework (test/e2e/framework/<pkg>) should not import the e2e test framework in order to avoid circular dependencies and to keep a clean interface.
A lof of the functions that are being moved over are of the form

func someMethod(f *framework.Framework) {
  ...
}

All these functions should be refactored to instead use the thing that is required from the test framework.
For example, a lof of code does something like f.ClientSet.CoreV1().ConfigMaps(f.Namespace.Name).Create(cm). This should be refactored to instead take a client interface and a namespace string as arguments to have ClientSet.CoreV1().ConfigMaps(Namespace).Create(cm) within the method.

/priority important-soon
/lgtm cancel
/milestone v1.17

test/e2e/node/apparmor.go

test/e2e/auth/pod_security_policy.go

test/e2e/node/apparmor.go

danielqsj · 2019-10-29T06:53:48Z

@alejandrox1 thanks for your review. It's a great guideline for this series of refactors.
PR updated, PTAL :)

mattjmcnaughton · 2019-10-29T12:33:53Z

Thank you very much for working on this @danielqsj !
Added a couple comments to guide the refactoring into the e2e test framework.
Overall, we want to include any code that may be useful for a user to test behaviour, if there is code that is specific to a test then the code should stay within the test.

A big thing is that function within an e2e test framework (test/e2e/framework/<pkg>) should not import the e2e test framework in order to avoid circular dependencies and to keep a clean interface.
A lof of the functions that are being moved over are of the form
func someMethod(f *framework.Framework) {
  ...
}
All these functions should be refactored to instead use the thing that is required from the test framework.
For example, a lof of code does something like f.ClientSet.CoreV1().ConfigMaps(f.Namespace.Name).Create(cm). This should be refactored to instead take a client interface and a namespace string as arguments to have ClientSet.CoreV1().ConfigMaps(Namespace).Create(cm) within the method.

/priority important-soon
/lgtm cancel
/milestone v1.17

Thanks for your insights here @alejandrox1 !

josiahbjorgaard · 2019-11-03T21:38:24Z

Hi, bug triage for v1.17 checking in to see if this PR will be merged before code freeze on Nov. 14?

danielqsj · 2019-11-07T03:19:38Z

/cc @oomichi

oomichi · 2019-11-08T22:45:03Z

test/e2e/framework/apparmor.go

@@ -23,7 +23,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/kubernetes/pkg/security/apparmor"
-	"k8s.io/kubernetes/test/e2e/framework"
 	e2epod "k8s.io/kubernetes/test/e2e/framework/pod"


This dependency way is not valid (e2e core framework to e2e framework subpackages(e2epod in this case).
I hesitate to merge this into the core framework at this time.

I also don't like this dependency here.
But if we looks into other files in framework, there are many place also has this dependency issue.
We need to sort out the calling relationship of each components of e2e framework.

@danielqsj

But if we looks into other files in framework, there are many place also has this dependency issue.

Yes, you are right.
Then we are trying to remove such invalid dependencies from the existing places.
I feel it is not necessary to add more.

@oomichi does one e2e framework subpackages has dependency of another e2e framework subpackage seems good ?
If it's good, I can move the apparmor.go to a dedicated subpackage like security.

But this will add the dependency of the package e2e/framework directly.

I opened an issue to track this issue: #85193

@danielqsj

@oomichi does one e2e framework subpackages has dependency of another e2e framework subpackage seems good ?

That is a nice point. IIUC we never discussed the dependency between subpackages.
I guess the dependency could be fine.

sure. Re-implemented this change by moving it to a new subpackage.
@oomichi Please help review it ? Thanks

josiahbjorgaard · 2019-11-13T04:05:20Z

@oomichi Do you think that this will be merged before v1.17 code freeze 11/14 5pm pacific time?

oomichi · 2019-11-13T18:37:50Z

@josiahbjorgaard

Do you think that this will be merged before v1.17 code freeze 11/14 5pm pacific time?

It's up to @danielqsj if we can see a PR which has clean dependency.

danielqsj · 2019-11-14T07:09:29Z

PR updated

oomichi · 2019-11-14T18:19:22Z

@danielqsj Thanks for updating, much better for me.

/lgtm
/approve

k8s-ci-robot · 2019-11-14T18:20:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielqsj, oomichi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [oomichi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested a review from pohly October 28, 2019 08:35

mattjmcnaughton approved these changes Oct 28, 2019

View reviewed changes

k8s-ci-robot assigned mattjmcnaughton Oct 28, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2019

alejandrox1 reviewed Oct 28, 2019

View reviewed changes

test/e2e/node/apparmor.go Outdated Show resolved Hide resolved

test/e2e/auth/pod_security_policy.go Outdated Show resolved Hide resolved

test/e2e/node/apparmor.go Outdated Show resolved Hide resolved

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Oct 28, 2019

k8s-ci-robot assigned alejandrox1 Oct 28, 2019

k8s-ci-robot added this to the v1.17 milestone Oct 28, 2019

k8s-ci-robot removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 28, 2019

danielqsj force-pushed the f-apparmor branch from 6e414b3 to 7d583c5 Compare October 29, 2019 06:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 29, 2019

k8s-ci-robot requested a review from oomichi November 7, 2019 03:19

oomichi reviewed Nov 8, 2019

View reviewed changes

danielqsj force-pushed the f-apparmor branch from 7d583c5 to 2aee2e3 Compare November 14, 2019 03:22

refactor apparmor utils in e2e

920dbdd

danielqsj force-pushed the f-apparmor branch from 2aee2e3 to 920dbdd Compare November 14, 2019 04:32

k8s-ci-robot assigned oomichi Nov 14, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2019

k8s-ci-robot merged commit 55cef80 into kubernetes:master Nov 14, 2019

danielqsj mentioned this pull request Nov 16, 2019

refactor events utils in e2e #85190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor apparmor utils in e2e #84439

refactor apparmor utils in e2e #84439

danielqsj commented Oct 28, 2019

mattjmcnaughton left a comment

alejandrox1 left a comment •

edited

danielqsj commented Oct 29, 2019

mattjmcnaughton commented Oct 29, 2019

josiahbjorgaard commented Nov 3, 2019

danielqsj commented Nov 7, 2019

oomichi Nov 8, 2019 •

edited

danielqsj Nov 9, 2019

oomichi Nov 12, 2019

danielqsj Nov 13, 2019

danielqsj Nov 13, 2019

danielqsj Nov 13, 2019

oomichi Nov 13, 2019

danielqsj Nov 14, 2019

josiahbjorgaard commented Nov 13, 2019

oomichi commented Nov 13, 2019

danielqsj commented Nov 14, 2019

oomichi commented Nov 14, 2019

k8s-ci-robot commented Nov 14, 2019

refactor apparmor utils in e2e #84439

refactor apparmor utils in e2e #84439

Conversation

danielqsj commented Oct 28, 2019

mattjmcnaughton left a comment

Choose a reason for hiding this comment

alejandrox1 left a comment • edited

Choose a reason for hiding this comment

danielqsj commented Oct 29, 2019

mattjmcnaughton commented Oct 29, 2019

josiahbjorgaard commented Nov 3, 2019

danielqsj commented Nov 7, 2019

oomichi Nov 8, 2019 • edited

Choose a reason for hiding this comment

danielqsj Nov 9, 2019

Choose a reason for hiding this comment

oomichi Nov 12, 2019

Choose a reason for hiding this comment

danielqsj Nov 13, 2019

Choose a reason for hiding this comment

danielqsj Nov 13, 2019

Choose a reason for hiding this comment

danielqsj Nov 13, 2019

Choose a reason for hiding this comment

oomichi Nov 13, 2019

Choose a reason for hiding this comment

danielqsj Nov 14, 2019

Choose a reason for hiding this comment

josiahbjorgaard commented Nov 13, 2019

oomichi commented Nov 13, 2019

danielqsj commented Nov 14, 2019

oomichi commented Nov 14, 2019

k8s-ci-robot commented Nov 14, 2019

alejandrox1 left a comment •

edited

oomichi Nov 8, 2019 •

edited